Check ancillary_variables in exclude_attrs before get_extra_ds #1140

zxdawn · 2020-04-13T07:24:19Z

I found this problem when I was trying to save tropomi scene to netcdf file.

There're two variables cited each other by ancillary_variables:

float cloud_radiance_fraction_nitrogendioxide_window(time=1, scanline=373, ground_pixel=450);
  :units = "1";
  :long_name = "Cloud radiance fraction at 440 nm for NO2 retrieval";
  :coordinates = "/PRODUCT/longitude /PRODUCT/latitude";
  :radiation_wavelength = 440.0f; // float
  :assumed_cloud_albedo = 0.8f; // float
  :ancillary_variables = "cloud_fraction_crb_nitrogendioxide_window /PRODUCT/SUPPORT_DATA/INPUT_DATA/cloud_pressure_crb";
  :_FillValue = 9.96921E36f; // float
  :_ChunkSizes = 1U, 373U, 450U; // uint

float cloud_fraction_crb_nitrogendioxide_window(time=1, scanline=373, ground_pixel=450);
  :proposed_standard_name = "effective_cloud_area_fraction_assuming_fixed_cloud_albedo";
  :units = "1";
  :long_name = "Cloud fraction at 440 nm for NO2 retrieval";
  :coordinates = "/PRODUCT/longitude /PRODUCT/latitude";
  :radiation_wavelength = 440.0f; // float
  :assumed_cloud_albedo = 0.8f; // float
  :ancillary_variables = "cloud_radiance_fraction_nitrogendioxide_window /PRODUCT/SUPPORT_DATA/INPUT_DATA/cloud_pressure_crb";
  :_FillValue = 9.96921E36f; // float
  :_ChunkSizes = 1U, 373U, 450U; // uint

I don't know this should be the bug of tropomi_l2 file or this is correct. This could cause the infinite recursion of get_extra_ds., like this:

  File "/yin_raid/xin/github/satpy/satpy/writers/cf_writer.py", line 160, in get_extra_ds
    ds_collection.update(get_extra_ds(ds))
  [Previous line repeated 988 more times]
  File "/yin_raid/xin/github/satpy/satpy/writers/cf_writer.py", line 159, in get_extra_ds
    for ds in dataset.attrs.get('ancillary_variables', []):
  File "/yin_raid/xin/miniconda3/envs/s5p/lib/python3.7/site-packages/xarray/core/dataarray.py", line 681, in attrs
    return self.variable.attrs
RecursionError: maximum recursion depth exceeded

Anyway, I decide to add ancillary_variables to exclude_attrs when using save_datasets for this issue. So, we need to check the exist of ancillary_variables before get_extra_ds.

ghost · 2020-04-13T07:24:35Z

Congratulations 🎉. DeepCode analyzed your code in 0.238 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard

Edit DeepCode’s Configurations here

satpy/writers/cf_writer.py

djhoese

I'm not sure this makes sense. extra will always be false if there are ancillary_variables which means that that for loop will never do anything. You've essentially just dropped the ancillary_variables attribute haven't you? Additionally, now these ancillary variables won't be included in the NetCDF file but we want them to be.

In my opinion this is both a bug in the tropomi data (does it even make scientific sense for the circular definition?) and satpy which could/should probably defend against this circular dependency.

Perhaps the "right" fix would be to include the currently processed dictionary to get_extra_ds so that it knows what it has already analyzed instead of doing it multiple times.

zxdawn · 2020-04-13T14:39:01Z

@djhoese Nice idea to let get_extra_ds check what it has already analyzed. (just like the similar function in tropomi_l2 reader)

BTW, I would ask someone in KNMI about the "wrong" ancillary_variables.

coveralls · 2020-04-14T03:10:03Z

Coverage increased (+0.4%) to 89.959% when pulling 195fb11 on zxdawn:save_dataset into 446af3a on pytroll:master.

codecov · 2020-04-21T12:56:44Z

Codecov Report

Merging #1140 into master will increase coverage by 0.35%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master    #1140      +/-   ##
==========================================
+ Coverage   89.60%   89.95%   +0.35%     
==========================================
  Files         200      208       +8     
  Lines       29484    30605    +1121     
==========================================
+ Hits        26420    27532    +1112     
- Misses       3064     3073       +9

Impacted Files	Coverage Δ
satpy/writers/cf_writer.py	`93.81% <50.00%> (+0.02%)`	⬆️
satpy/tests/writer_tests/test_cf.py	`98.62% <100.00%> (+0.08%)`	⬆️
satpy/readers/tropomi_l2.py	`87.06% <0.00%> (-2.94%)`	⬇️
satpy/readers/avhrr_l1b_gaclac.py	`93.33% <0.00%> (-1.34%)`	⬇️
satpy/readers/viirs_compact.py	`64.12% <0.00%> (-1.27%)`	⬇️
satpy/readers/hrit_jma.py	`97.94% <0.00%> (-1.22%)`	⬇️
satpy/tests/test_composites.py	`99.88% <0.00%> (-0.12%)`	⬇️
satpy/writers/geotiff.py	`90.74% <0.00%> (ø)`
satpy/tests/test_dataset.py	`100.00% <0.00%> (ø)`
satpy/readers/netcdf_utils.py	`100.00% <0.00%> (ø)`
... and 42 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 446af3a...195fb11. Read the comment docs.

mraspaud

Nice catch, thanks for the pr. Could you add a small test for this?

zxdawn · 2020-06-04T07:39:48Z

satpy/tests/writer_tests/test_cf.py

+            scn.save_datasets(filename=filename, writer='cf')
+            with xr.open_dataset(filename) as f:
+                self.assertEqual(f['test-array-1'].attrs['ancillary_variables'],
+                                 'test-array-2')


As mentioned in the issue in xarray, ancillary_variables isn't decoded.
So, since save_datasets succeeds, I just check the name here.

mraspaud

Just a small suggestion (docstring style), otherwise looks good to me.

satpy/tests/writer_tests/test_cf.py

zxdawn · 2020-12-24T13:02:02Z

I suppose this PR is fine enough for checking ancillary_variables.

djhoese

Looks good to me. Thanks.

zxdawn added 2 commits April 13, 2020 14:46

Check ancillary_variables in exclude_attrs

15f1198

Bugfix: add the option of extra

cd9bcda

zxdawn requested a review from mraspaud as a code owner April 13, 2020 07:24

Delete own test

4ec6a87

stickler-ci reviewed Apr 13, 2020

View reviewed changes

satpy/writers/cf_writer.py Outdated Show resolved Hide resolved

zxdawn mentioned this pull request Apr 13, 2020

save_datasets doesn't work for tropomi_l2 data #1143

Closed

djhoese requested changes Apr 13, 2020

View reviewed changes

Check whether ancillary_variables has been processed

f9fffd5

gerritholl mentioned this pull request May 6, 2020

RecursionError if loading ancillary variable raises KeyError #1182

Open

mraspaud requested changes Jun 2, 2020

View reviewed changes

Add test_ancillary_variables

8c2cab8

zxdawn commented Jun 4, 2020

View reviewed changes

Better docstring

195fb11

mraspaud reviewed Jun 4, 2020

View reviewed changes

satpy/tests/writer_tests/test_cf.py Outdated Show resolved Hide resolved

djhoese approved these changes Dec 24, 2020

View reviewed changes

djhoese merged commit 694f2c0 into pytroll:master Jan 4, 2021

djhoese mentioned this pull request May 25, 2023

Refactor CFWriter.save_datasets and enable retrieval of equivalent xr.Dataset with scn.to_xarray() #2259

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check ancillary_variables in exclude_attrs before get_extra_ds #1140

Check ancillary_variables in exclude_attrs before get_extra_ds #1140

zxdawn commented Apr 13, 2020 •

edited

Loading

ghost commented Apr 13, 2020 •

edited by ghost

Loading

djhoese left a comment

zxdawn commented Apr 13, 2020

coveralls commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 21, 2020 •

edited

Loading

mraspaud left a comment

zxdawn Jun 4, 2020

mraspaud left a comment

zxdawn commented Dec 24, 2020

djhoese left a comment

Check ancillary_variables in exclude_attrs before get_extra_ds #1140

Check ancillary_variables in exclude_attrs before get_extra_ds #1140

Conversation

zxdawn commented Apr 13, 2020 • edited Loading

ghost commented Apr 13, 2020 • edited by ghost Loading

👉 View analysis in DeepCode’s Dashboard

Edit DeepCode’s Configurations here

djhoese left a comment

Choose a reason for hiding this comment

zxdawn commented Apr 13, 2020

coveralls commented Apr 14, 2020 • edited Loading

codecov bot commented Apr 21, 2020 • edited Loading

Codecov Report

mraspaud left a comment

Choose a reason for hiding this comment

zxdawn Jun 4, 2020

Choose a reason for hiding this comment

mraspaud left a comment

Choose a reason for hiding this comment

zxdawn commented Dec 24, 2020

djhoese left a comment

Choose a reason for hiding this comment

zxdawn commented Apr 13, 2020 •

edited

Loading

ghost commented Apr 13, 2020 •

edited by ghost

Loading

coveralls commented Apr 14, 2020 •

edited

Loading

codecov bot commented Apr 21, 2020 •

edited

Loading